feat: Display Unlimited for really large spending caps on Permit #29102
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
pedronfigueiredo
changed the title
pedronfigueiredo
force-pushed
the
pnf/3763
branch
from
abc6a9a to
1448df0
Compare
Builds ready [1448df0]
Page Load Metrics (1792 ± 119 ms)
Bundle size diffs
|
jpuri
reviewed
Dec 12, 2024
| truncatedStartChars: 15, | ||
| truncatedEndChars: 0, | ||
| skipCharacterInEnd: true, | ||
| })} |
There was a problem hiding this comment.
@pedronfigueiredo : not all decoded simulation are Spending Cap some are even amount that user will receive. Can you plz confirm with if this change is ok for all state change types in decoded simulations.
pedronfigueiredo
force-pushed
the
pnf/3763
branch
2 times, most recently
from
161d49a to
e68c588
Compare
Builds ready [e68c588]
Page Load Metrics (1723 ± 81 ms)
Bundle size diffs
|
pedronfigueiredo
force-pushed
the
pnf/3763
branch
from
880f77c to
41c27cc
Compare
Builds ready [41c27cc]
Page Load Metrics (1815 ± 82 ms)
Bundle size diffs
|
pedronfigueiredo
force-pushed
the
pnf/3763
branch
2 times, most recently
from
c88c37d to
359cb29
Compare
Builds ready [359cb29]
Page Load Metrics (1584 ± 33 ms)
Bundle size diffs
|
jpuri
reviewed
Dec 17, 2024
| @@ -10,7 +10,7 @@ import { formatAmount } from '../../../../simulation-details/formatAmount'; | |||
| import { useDecodedTransactionData } from '../../hooks/useDecodedTransactionData'; | |||
| import { useIsNFT } from './use-is-nft'; | |||
|
|
|||
| const UNLIMITED_THRESHOLD = 10 ** 15; | |||
| export const UNLIMITED_THRESHOLD = 10 ** 15; | |||
There was a problem hiding this comment.
It will be better to move the constant to a constants file.
jpuri
reviewed
Dec 17, 2024
| return { | ||
| tokenValue: formatAmount('en-US', tokenAmount), | ||
| tokenValueMaxPrecision: formatAmountMaxPrecision('en-US', tokenAmount), | ||
| shouldShowUnlimitedValue: Number(value) > UNLIMITED_THRESHOLD, |
There was a problem hiding this comment.
shouldShowUnlimitedValue will be Number(value) > UNLIMITED_THRESHOLD && canDisplayValueAsUnlimited .
We do not gain a lot by memoising it, but if we choose to do so it will be better to memoise whole of Number(value) > UNLIMITED_THRESHOLD && canDisplayValueAsUnlimited here I think.
pedronfigueiredo
force-pushed
the
pnf/3763
branch
from
359cb29 to
fe64529
Compare
Builds ready [fe64529]
Page Load Metrics (1916 ± 93 ms)
Bundle size diffs
|
pedronfigueiredo
force-pushed
the
pnf/3763
branch
from
fe64529 to
31bd740
Compare
Builds ready [31bd740]
Page Load Metrics (1703 ± 116 ms)
Bundle size diffs
|
jpuri
previously approved these changes
Dec 17, 2024
matthewwalsh0
previously approved these changes
Dec 18, 2024
There was a problem hiding this comment.
Could we extend ui/pages/confirmations/components/confirm/info/shared/constants.ts which also has a single constant in? 😄
| @@ -0,0 +1 @@ | |||
| export const UNLIMITED_THRESHOLD = 10 ** 15; | |||
There was a problem hiding this comment.
Minor, maybe TOKEN_VALUE_UNLIMITED_THRESHOLD for clarity?
| tokenValue: formatAmount('en-US', tokenAmount), | ||
| tokenValueMaxPrecision: formatAmountMaxPrecision('en-US', tokenAmount), | ||
| shouldShowUnlimitedValue: | ||
| canDisplayValueAsUnlimited && Number(value) > UNLIMITED_THRESHOLD, |
There was a problem hiding this comment.
Why do we have to cast, what other type could it be?
There was a problem hiding this comment.
value can be both a number or a string
digiwand
previously approved these changes
Dec 18, 2024
| @@ -56,6 +56,9 @@ type PermitSimulationValueDisplayParams = { | |||
|
|
|||
| /** True if value is being debited to wallet */ | |||
| debit?: boolean; | |||
|
|
|||
| /** Wether a large amount can be substituted by "Unlimited" */ | |||
There was a problem hiding this comment.
Suggested change
| /** Wether a large amount can be substituted by "Unlimited" */ | |
| /** Whether a large amount can be substituted by "Unlimited" */ |
pedronfigueiredo
dismissed stale reviews from digiwand, matthewwalsh0, and jpuri
via
da8f3d1
pedronfigueiredo
force-pushed
the
pnf/3763
branch
from
31bd740 to
da8f3d1
Compare
Builds ready [da8f3d1]
Page Load Metrics (1412 ± 27 ms)
Bundle size diffs
|
jpuri
approved these changes
Dec 19, 2024
digiwand
approved these changes
Dec 19, 2024
metamaskbot
added
the
release-12.11.0
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Uses
UNLIMITED_THRESHOLDto determine wether or not to show a permit amount as "Unlimited". Updates unit tests.Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3763
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist